Preserve generic SQS listener payload types - #1666
Conversation
|
Hi, everyone! How are you? Are there any updates on this PR? Please let me know if you need any additional information from me. I’m looking forward to this feature because I’d like to use it in a project at my company. 😊 Thank you, and have a lovely day! |
tomazfernandes
left a comment
There was a problem hiding this comment.
@brun0-4ugusto thanks for the PR, looking good.
Left a few comments around backwards compatibility for existing overrides and keeping the delegation between the old and new overloads one-directional.
| * target type. Note that type mappers in MessagingMessageConverters take precedence over this type. | ||
| * @param payloadDeserializationType the target class | ||
| */ | ||
| public void setPayloadDeserializationType(@Nullable Class<?> payloadDeserializationType) { |
There was a problem hiding this comment.
Let's have this method delegate to the new overload with a null conversion hint.
That way both setters converge on the 3-arg hook, whose default already delegates to this 2-arg hook, so delegation flows in one direction and existing 2-arg overrides remain invoked from every entry point.
| protected void doConfigurePayloadTypeOnContext(Class<?> payloadType, MessageConversionContext context) { | ||
| ConfigUtils.INSTANCE.acceptIfInstance(context, SqsMessageConversionContext.class, | ||
| ctx -> ctx.setPayloadClass(payloadType)); | ||
| doConfigurePayloadTypeOnContext(payloadType, null, context); |
There was a problem hiding this comment.
For backwards compatibility, let's invert the delegation here: keep the payload class assignment in the 2-arg override, and have the 3-arg override call it before setting the hint.
| @@ -190,6 +193,21 @@ public void setPhase(int phase) { | |||
| */ | |||
| public void setPayloadDeserializationType(@Nullable Class<?> payloadDeserializationType) { | |||
| this.payloadDeserializationType = payloadDeserializationType; | |||
There was a problem hiding this comment.
Let's delegate to setPayloadDeserializationType with a null conversion hint, and have the 2-arg overload assign both fields directly instead of calling this method.
That makes the richer overload the single write site, so both fields are updated together.
| @@ -40,4 +40,20 @@ public interface MethodPayloadTypeInferrer { | |||
| @Nullable | |||
There was a problem hiding this comment.
Let's deprecate this method, routing implementations to the new method instead.
|
Thank you for the feedback @tomazfernandes ! I really appreciated the points you raised in the review. I’m already working on the requested changes, and I’ll update the PR with the fixes later this week. |
Pass listener MethodParameter metadata through the container and message source as a SmartMessageConverter conversion hint so Jackson can resolve complete generic payload types. Add unit, converter, batch, and LocalStack integration coverage for wrappers and nested collections. Fixes awspringgh-1597
6e8a166 to
22eb49d
Compare
|
Hi @tomazfernandes ! Thanks again for the review. I’ve addressed the comments and updated the PR accordingly. Regarding MethodPayloadTypeInferrer, I deprecated I intentionally kept Could you please take another look when you have a chance? In particular, I’d appreciate your feedback on whether preserving the existing SAM is the direction you had in mind, or whether you would prefer changing it despite the compatibility impact. Thank you! :) |
📢 Type of change
📜 Description
This PR fixes generic payload deserialization for
@SqsListenermethods.Previously, payload type inference retained only the raw
Class<?>. Types such asGenericWrapper<TestEvent>andMessage<List<TestEvent>>were therefore reduced toGenericWrapper.classandList.class. Without the complete generic type, Jackson deserialized nested values asLinkedHashMapinstances.This change:
MethodPayloadMetadata, containing the raw payload class and an optional conversion hint.MethodParameteras theSmartMessageConverterconversion hint.MessageConversionContext.SmartMessageConverter#fromMessage(Message, Class, Object)when a conversion hint is available.MessageConvertercontract for converters that do not implementSmartMessageConverter.GenericWrapper<TestEvent>GenericWrapper<List<TestEvent>>Message<List<TestEvent>>List<GenericWrapper<TestEvent>>List<Message<GenericWrapper<TestEvent>>>MethodPayloadTypeInferrerimplementations source-compatible through a default metadata adaptation method.MessageSourceJavadoc to describe where payload conversion occurs.💡 Motivation and Context
@SqsListenerpayload deserialization currently loses generic type information because the inferred listener type is transported only as a rawClass<?>.As a result, Jackson cannot determine the concrete type of generic fields or collection elements and falls back to
LinkedHashMap.The problem is also visible before listener invocation because SQS payload conversion happens at the
MessageSourcelevel. Therefore, interceptors, error handlers, and acknowledgement callbacks may also receive incorrectly typed generic values.Spring's
SmartMessageConverteralready supports a conversion hint. Its base converters can use aMethodParameterhint to recover the complete genericType, so this PR propagates that existing Spring metadata instead of introducing custom Jackson-specific type resolution.Fixes #1597
💚 How did you test it?
Added and executed focused tests covering:
MethodPayloadTypeInferrerimplementations.SmartMessageConverterhint invocation and regularMessageConverterfallback.Validation results:
SqsPayloadTypeInferenceIntegrationTestspassed with LocalStack.Commands used:
./mvnw -pl spring-cloud-aws-sqs -am testFull module: Tests run: 658, Failures: 0, Errors: 0, Skipped: 5📝 Checklist
🔮 Next steps
Subject to maintainer feedback, a separate follow-up PR could expand generic payload inference for listener methods inherited from generic superclasses.
For example:
When Spring discovers the listener method, its original declaration still describes the payload as GenericWrapper. To deserialize it as GenericWrapper, the MethodParameter must also be resolved against the concrete listener class (TestEventListener). This allows Spring's type resolution infrastructure to substitute T with TestEvent before passing the conversion hint to the SmartMessageConverter.
I have already explored and implemented this extension in a separate branch, including tests for inherited generic listener methods and nested or batch generic payload shapes. It is deliberately not included in this PR so that the initial fix remains focused and easier to review.
We can open a discussion about the expected scope and compatibility requirements for inherited generic listeners. If the maintainers agree with the direction, the existing implementation can be refined and submitted as a separate follow-up PR.